Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ntuple] Add a common RNTupleOpenSpec #16653

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

enirolf
Copy link
Contributor

@enirolf enirolf commented Oct 11, 2024

The specification structs RNTupleReader::ROpenSpec and RNTupleSourceSpec (used by RNTupleProcessor) are virtually identical, so it makes no sense to keep both around. This PR adds a common RNTupleOpenSpec (other naming suggestions welcome of course) to RNTupleUtil that can be used by both RNTupleReader::OpenFriends and the RNTupleProcessor.

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a minor comment

std::string fStorage;
RNTupleReadOptions fOptions;

RNTupleOpenSpec() = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add rule of five methods

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to correct myself here, the default constructor is not involved with the rule of five, so actually it would be better to not write any other of the special methods in the class. Apologies for the confusion!

Copy link

github-actions bot commented Oct 11, 2024

Test Results

    17 files      17 suites   3d 23h 14m 46s ⏱️
 2 713 tests  2 712 ✅ 0 💤 1 ❌
43 511 runs  43 509 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 313445a.

♻️ This comment has been updated with latest results.

@enirolf enirolf force-pushed the ntuple-openspec branch 4 times, most recently from d0722a6 to 8ee2533 Compare October 14, 2024 14:43
The specification structs `RNTupleReader::ROpenSpec` and
`RNTupleSourceSpec` (used by `RNTupleProcessor`) are virtually
identical, so it makes no sense to keep both around.
@enirolf enirolf merged commit 5b4268d into root-project:master Oct 14, 2024
17 of 20 checks passed
@enirolf enirolf deleted the ntuple-openspec branch October 15, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants